Skip to content
This repository was archived by the owner on Dec 21, 2021. It is now read-only.

Conversation

@timoxley
Copy link
Contributor

@timoxley timoxley commented Sep 16, 2020

Co-locating and re-jigging publishing step.

Also:

  • Fixes non-functional MessageCreationUtil tests.
  • Swaps out receptacle for more general purpose & reusable mem, p-memoize & quick-lru
  • Removes expensive ethers.Wallet.createRandom() call in tests.
  • Tidied up login endpoint tests.
  • Removed all use of ensureConnected/ensureDisconnected.

Builds on:

@timoxley timoxley changed the base branch from master to promisify-subscription-methods September 16, 2020 11:52
@timoxley timoxley requested review from harbu, hpihkala and mirpo and removed request for mirpo September 17, 2020 15:35
@timoxley timoxley force-pushed the promisify-subscription-methods branch from 2615d55 to d73804d Compare September 18, 2020 16:58
Copy link
Contributor

@mirpo mirpo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

src/utils.js Outdated
}

/* eslint-disable object-curly-newline */
export function CacheAsyncFn(fn, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@harbu harbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

src/Publisher.js Outdated
constructor(client) {
this.client = client
const cacheOptions = client.options.cache
this.msgChainer = new OrderedMessageChainCreator(cacheOptions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructor of OrderedMessageChainCreator takes no arguments?

…event backdated messages silently breaking future publishes (#166)

* Improve authFetch logging.

* Update message sequencer to strictly enforce message order.

* Queue publishes per-stream otherwise can skip forward then back even when messages published in correct sequence.

* Add partial solution to broken backdated messages, at least doesn't break regular sequential publishes.

* Tidy up, add some comments.

* Move publish queue fn into utils.
@timoxley timoxley merged commit 0fdcba3 into promisify-subscription-methods Nov 24, 2020
@timoxley timoxley deleted the refactor-publish branch November 24, 2020 15:51
timoxley added a commit that referenced this pull request Nov 24, 2020
* Promisify subscribe/unsubscribe.

* Fail _request call if unsubscribe/subscribe request fails to send.

* Fix missing import in resend test.

* Fix browser tests.

* Clean up unused function in test.

* 4. Refactor Publish (#164)

* Refactor & co-locate publish code.

* Avoid using expensive ethers.Wallet.createRandom() calls in test. e.g. 1000x calls with ethers: 14s, randomBytes: 3.5ms.

* Ensure messageCreationUtil is cleaned up after test.

* Fix non-functional MessageCreationUtil test.

* Swap out receptacle for more flexible mem/p-memoize/quick-lru.

* Convert LoginEndpoints test to async/await.

* Remove calls to ensureConnected/ensureDisconnected in test.

* 5. Message Sequencing – Guarantee sequence follows publish order & Prevent backdated messages silently breaking future publishes (#166)

* Improve authFetch logging.

* Update message sequencer to strictly enforce message order.

* Queue publishes per-stream otherwise can skip forward then back even when messages published in correct sequence.

* Add partial solution to broken backdated messages, at least doesn't break regular sequential publishes.

* Tidy up, add some comments.

* Move publish queue fn into utils.
@timoxley timoxley mentioned this pull request Nov 24, 2020
@timoxley timoxley mentioned this pull request Jan 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants